Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add if_not_exists to create_view and if_exists to drop_view #382

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

serg-kovalev
Copy link

@serg-kovalev serg-kovalev commented Feb 8, 2023

Provide an ability to pass if_not_exists param for view creation. It's handy to have it when you disable_ddl_transaction in Rails migration and add indexes concurrently.
And similar functionality with if_exists param to drop a view.

For instance we have a migration like that:

class CreateSomeView < ActiveRecord::Migration[7.0]
  disable_ddl_transaction!

  def up
    create_view :mv_some_name, materialized: true

    safety_assured do
      add_index :mv_some_name, :receipt_uuid, unique: true, algorithm: :concurrently
      add_index :mv_some_name, :invoice_number, algorithm: :concurrently
    end
  end

  def down
    drop_view :mv_some_name, materialized: true
  end
end

As it's recommended to update indexes concurrently out of a transaction, those operations may accidentally fail. It happens very rarely, but we faced it several times. As a result, migration will stuck and it will require manual resolution.

The change that I made doesn't bring some level of uncertainty, I think. As by default, we do everything like we did before, but if we really need to make it a bit flexible, we now can do that.

Please let me know if it makes sense.
Thank you in advance!

@serg-kovalev serg-kovalev force-pushed the feature/add-exist-check-on-view-creation branch from 380eccf to e145024 Compare February 8, 2023 13:30
Copy link

@hound hound bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some files could not be reviewed due to errors:

Warning: unrecognized cop Layout/ParameterAlignment found in .rubocop.yml
Warning: unrecognized cop Layout/ParameterAlignment found in .rubocop.yml
Warning: unrecognized cop Layout/AssignmentIndentation found in .rubocop.yml
Warning: unrecognized cop Lint/DuplicateHashKey found in .rubocop.yml
Error: Unknown Ruby version 2.7 found in `TargetRubyVersion` parameter (in .rubocop.yml).
Supported versions: 2.1, 2.2, 2.3, 2.4, 2.5

@serg-kovalev serg-kovalev force-pushed the feature/add-exist-check-on-view-creation branch from 210d552 to f4da7c1 Compare February 8, 2023 13:46
@serg-kovalev
Copy link
Author

@derekprior Hi! Hope you are well.
Did not talk to you since we worked on NO DATA functionality.
Could you please take a look at this PR? What do you think about adding functionality similar to what Rails has?

@serg-kovalev serg-kovalev force-pushed the feature/add-exist-check-on-view-creation branch 2 times, most recently from 6b76770 to 5863bee Compare February 8, 2023 13:56
@derekprior
Copy link
Contributor

Can you say more about this?

It's handy to have it when you disable_ddl_transaction in Rails migration and add indexes concurrently

Conceptually, one of the ideas of Scenic is that careful management of your schema using Scenic means you have certainty around your database state. IF NOT EXISTS implies uncertainty.

@serg-kovalev
Copy link
Author

serg-kovalev commented Feb 9, 2023

Can you say more about this?

It's handy to have it when you disable_ddl_transaction in Rails migration and add indexes concurrently

Conceptually, one of the ideas of Scenic is that careful management of your schema using Scenic means you have certainty around your database state. IF NOT EXISTS implies uncertainty.

@derekprior Sure!
For instance we have a migration like that:

class CreateSomeView < ActiveRecord::Migration[7.0]
  disable_ddl_transaction!

  def up
    create_view :mv_some_name, materialized: true

    safety_assured do
      add_index :mv_some_name, :receipt_uuid, unique: true, algorithm: :concurrently
      add_index :mv_some_name, :invoice_number, algorithm: :concurrently
    end
  end

  def down
    drop_view :mv_some_name, materialized: true
  end
end

As it's recommended to update indexes concurrently out of a transaction, those operations may accidentally fail. It happens very rarely, but we faced it several times. As a result, migration will stuck and it will require manual resolution.

The change that I made doesn't bring some level of uncertainty, I think. As by default, we do everything like we did before, but if we really need to make it a bit flexible, we now can do that.

Please let me know if it makes sense.
Thank you in advance

@serg-kovalev serg-kovalev force-pushed the feature/add-exist-check-on-view-creation branch from 5863bee to b401623 Compare February 14, 2023 09:39
@serg-kovalev
Copy link
Author

@derekprior Hey! Hope you are doing well. Have you had a chance to take a look?

@calebhearth calebhearth added this to the 2.0 milestone Jan 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants